-
Notifications
You must be signed in to change notification settings - Fork 659
test(rome_js_analyze): every rule has its folder #4318
Conversation
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Assuming that orphan snapshots is just an issue of What's the change around JSON? I can't understand the argument around the diagnostics |
In my view it makes new contributor entrance easier because there is a single way to start writing tests. Moreover, it makes easier to identify in a tree view where the tests are located since everything is in a folder.
Baically with a js file you get the following snpshot: # Input
```js
let x = 1; foo(x)
let x = 1; x++;
for (let i in [1,2,3]) { foo(i); }
```
# Diagnostics
```
invalid.js:1:1 lint/style/useConst FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! This 'let' declares a variable which is never re-assigned.
> 1 │ let x = 1; foo(x);
│ ^^^
i 'x' is never re-assigned.
> 1 │ let x = 1; foo(x);
│ ^
invalid.js:5:6 lint/style/useConst FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! This 'let' declares a variable which is never re-assigned.
> 3 │ for (let i in [1,2,3]) { foo(i); }
│ ^^^
i 'i' is never re-assigned.
> 1 │ for (let i in [1,2,3]) { foo(i); }
│ ^
``` While with jsonc, you get the following snapshot: # Input
```js
let x = 1; foo(x);
```
# Diagnostics
```
invalid.jsonc:1:1 lint/style/useConst FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! This 'let' declares a variable which is never re-assigned.
> 1 │ let x = 1; foo(x);
│ ^^^
i 'x' is never re-assigned.
> 1 │ let x = 1; foo(x);
│ ^
```
# Input
```js
let x = 1; x++;
```
# Input
```js
for (let i in [1,2,3]) { foo(i); }
```
# Diagnostics
```
invalid.jsonc:1:6 lint/style/useConst FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! This 'let' declares a variable which is never re-assigned.
> 1 │ for (let i in [1,2,3]) { foo(i); }
│ ^^^
i 'i' is never re-assigned.
> 1 │ for (let i in [1,2,3]) { foo(i); }
│ ^
``` It is easier to catch in the second form that an input does not trigger the rule, and thus the test is either in the wrong file (in |
I think there's something wrong with the code blocks, both have diagnostics coming from a I would argue that having a JS file is better with IDE experience, because you can see if you write wrong code. |
Fixed. They are "hand-made" examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have to change the test runner to make sure that only folders are allowed?
Summary
This PR normalizes the folder structure of linter tests: every rule has a dedicated folder.
Moreover, I caught a copy-paste error:
correctness/noDuplicateObjectKeys.js.snap
has been moved tosuspicious/noDuplicateObjectKeys.js.snap
. However, the original file was not removed. This kind of error should be easier to catch with the new structure.I also turn some files into JSONC. This is easier to catch a potential missing diagnostic since the code and the diagnostic are close.
I also updated the CONTRIBUTING file to encourage the use of folder. By the way, we could change the test script to require it...
Test Plan
Documentation
CONTRIBUTING updated.